Repository navigation
Conversation
Give a datanode the ability to decode and execute a plan whose MergeScan carries a nested plan (e.g. an inner MergeScan over a build table), so a distributed sub-plan pushed to a datanode can itself fan out to other datanodes. This is the execution/codec capability only; it is not wired to any user-facing switch and produces no plan by itself. Codec (src/query/src/query_engine/default_serializer.rs): - MergeScanAwareSerializer keeps the request's SessionState and the engine catalog manager, and decodes a MergeScan payload's tables by (catalog, schema, table) name through the engine catalog rather than the region-aware request catalog list (which binds every table to the request's region and breaks on mismatched columns). Datanode (src/datanode): - New DatanodeRegionQueryHandler (region_query.rs) serves region queries for the MergeScan nodes of a received plan, resolving region leaders via PartitionRuleManager and dispatching to peer datanodes. - Wire KvBackendCatalogManager + PartitionRuleManager + NodeClients into the datanode's catalog/cache registry (with_dist_planner stays false: a datanode never rewrites its own plan). Bug fixes uncovered while wiring this up: - cache registry: register the table/table_info/table_name/table_route/ partition_info caches the datanode's catalog manager and partition rule manager require (previously missing -> panic on lookup). - default_serializer: decode_sub_plan bridge no longer drives a future on a current-thread runtime from within another executor (spawn a dedicated thread + runtime when not on a multi-thread runtime). Tested by query_engine::default_serializer unit tests: nested MergeScan round-trip and engine-catalog resolution with mismatched column names. No user-facing behavior change. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
… state SessionStateBuilder::new_from_existing collects the function registry (HashMap) into a Vec and build() re-registers each entry; register_udf registers aliases before the function's own name and the Vec order is unstable. Rebuilding the session state to install MergeScanAwareSerializer after the GreptimeDB functions were registered could therefore rebind a name/alias (e.g. date_format, an alias of DataFusion's to_char) back to the built-in function. This path runs for every decoded plan, not only nested MergeScan ones. Extract the GreptimeDB function registration into register_greptime_functions and re-apply it after the final build of both the top-level decode state and the MergeScan payload state. Add regression tests asserting date_format stays bound to the GreptimeDB implementation on both paths. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Three follow-ups from review of the nested MergeScan execution capability: 1. Layer the datanode caches (P2b). table_cache derives from table_info_cache + table_name_cache, and partition_info_cache derives from table_route_cache, but they were all registered in a single flat CacheRegistry whose invalidate() runs concurrently with no ordering. A derived cache could be invalidated and refilled from a base cache that had not been invalidated yet, leaving stale metadata. Build the base caches and the derived caches as separate layers of a LayeredCacheRegistry (reusing the existing layering mechanism) so base caches finish invalidating before derived ones. Both the production path (cmd) and the test scaffolding now build the same layered registry; the flat build_datanode_cache_registry is removed. 2. Don't let a nested MergeScan's internal region query compete for the concurrency permit (P2a). An outer query holds its permit until its stream is fully consumed, and a nested MergeScan makes the datanode issue region queries to peer datanodes as execution stages of that same query. If such an inner stage also took a permit, the outer query waiting on it would deadlock against its own held permit (or time out when max_concurrent_queries > 0). Mark inner DN->DN region queries as internal stages: QueryRequest gains an flag carried over the wire in the request header's query_context extensions (reserved key, not injectable via hints), and the region server skips acquiring a concurrency permit for internal-stage requests on both acquire sites. Outer queries still acquire normally. 3. Add an end-to-end capability test (tests-integration/.../nested_merge_scan_capability_test.rs): hand-build Join(<local probe region>, MergeScan(build regions on both datanodes)) with mismatched column names and duplicate build keys, ship it to a datanode over a real TCP Flight endpoint, consume the stream fully and compare the row multiset against the frontend's reference join result, assert a real cross-datanode DoGet via an RPC counter, and assert an inner region failure fails the whole query. Also migrate the with_real_datanode_grpc_addr + DatanodeRpcStats test scaffolding. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…on fork ScalarUDFImpl's trait bounds changed so its impl can no longer be downcast via as_any(); assert the function equals the GreptimeDB implementation instead, which to_char can never satisfy. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Add an end-to-end regression test for the nested MergeScan capability under max_concurrent_queries = 1: both datanodes' only permit is held by the outer queries' streams while their inner MergeScans fetch the build regions from the same datanodes over real Flight connections. The result consumption is bounded by a timeout, so a broken link anywhere in the chain (marking the inner request internal, carrying the marker in the request header, honoring it at the receiving acquire sites) fails the test instead of deadlocking. Also add a minimal with_datanode_options_override hook to the cluster builder, and fix the test module doc: the outer request is handed to RegionServer::handle_remote_read in process; only the inner region queries go over the wire. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
The table route cache uses InitStrategy::VersionChecked, whose version counter belongs to the whole CacheContainer rather than to a single table: any TableId invalidation bumps the shared version, so a cold load of one table's route retries when an unrelated table is invalidated concurrently. Document this as a known limitation on the region query handler, and pin the behavior with an integration test: run the nested plans while a background task invalidates an unrelated table id on every datanode, assert the results still match the expected multiset (retries only add work, they never return a stale route), and report the observed load amplification via the cache miss counter (2 loads without invalidations, 8 over three runs with 24 unrelated invalidations in this run). Also expose the datanode cache registries on the test cluster so a test can drive the local invalidation path that the heartbeat handler drives after a metasrv broadcast. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…join Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…ss support Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
e9763f6 to
694489f
Compare
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…tures Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
|
Warning @discord9 has 9 open non-draft pull requests in this repository, over the limit of 5. Review is the scarcest resource here. Please land or close some of these before
This check is advisory for now and blocks nothing. |
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
The default query path must not receive row statistics that could flip DataFusion JoinSelection from Partitioned to CollectLeft. Only compute row estimates when the session explicitly opts in with SET experimental_dist_join = true. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
dist_join.result: the default (non-opt-in) physical plan now stays a partitioned hash join (RepartitionExec on both sides) after row estimates are only computed when experimental_dist_join is enabled; drop the CollectLeft-shaped expectation recorded while estimates were injected unconditionally. explain.result: DistJoinPlanner is a registered analyzer rule, so EXPLAIN VERBOSE now prints its 'logical_plan after DistJoinPlanner' stage (same pattern as DistPlannerAnalyzer/JsonSchemaConcretizeRule). Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…t join Reviewer findings on the opt-in nested broadcast join: P1: the rewritten plan re-executes the SMALL-side subplan on every BIG-side region, so volatile expressions (e.g. random()) on the SMALL side change result semantics within one query. Add a volatility guard: contains_volatile_expr inspects the stripped small-side plan (projections, filters, table-scan filters, recursively) and refuses the rewrite. Conservative fallback only; a shared-materialization implementation is a separate stack PR. P2: experimental_dist_join never crossed the QueryContext protobuf roundtrip, so the datanode's nested MergeScan planning always skipped row estimates and JoinSelection could flip to BIG-build when only the append-only BIG side had exact statistics. Follow the read_preference extension-key precedent: encode query.experimental_dist_join in the protobuf extensions when enabled, restore on decode. Tests: volatile-small-side rejection unit test, protobuf roundtrip unit tests, and a real-cluster regression test asserting every dispatched BIG region's HashJoinExec builds the 2-row SMALL side and probes the append-only BIG side. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
The P1 guard rejected volatile expressions inside the small-side sub-plan but missed volatile expressions that appear only in join.on keys. ExtractEquijoinPredicate can extract volatile expressions into join keys (e.g. `ON b.k = CASE WHEN random() < 0.5 THEN s.k ELSE s.k + 1 END`), and the physical-plan-time Projection that computes such keys would be replicated per BIG region after the broadcast rewrite. Extend the guard to also check small-side join key expressions for volatility, and add a unit test covering this shape. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
Declare the volatile test UDF as non-nullable via return_field_from_args so FilterNullJoinKeys does not inject a volatile IS-NOT-NULL filter into the small sub-plan. Add assertions that verify the test premise: the small plan has no volatile expressions and the volatile expression exists only in the JOIN key, ensuring the test exercises the join-key guard specifically. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
fengjiachun
left a comment
There was a problem hiding this comment.
Two inline findings from static review of this head. No local builds or tests were run.
| .collect(), | ||
| join.filter.clone(), | ||
| join.join_type, | ||
| join.join_constraint, |
There was a problem hiding this comment.
Please check that the rewritten payload is serializable before moving the join into MergeScan. An INNER JOIN ... USING (k) has non-empty equijoin keys and can pass these guards when the table statistics favor broadcasting, but this preserves JoinConstraint::Using. The pinned DataFusion Substrait producer explicitly rejects that constraint (producer/rel/join.rs:31–34). RegionRequester::handle_query then propagates the encoding error, so enabling the optimization makes a previously executable query such as SELECT b.k FROM big b INNER JOIN small s USING (k) fail.
The earlier DistPlannerAnalyzer encoding check does not cover the new remote payload created here. Could we retain the original plan if the rewritten payload cannot be encoded, and add an off/on SQL execution regression for USING? Explicitly excluding Using would address this case, while checking the whole payload would also cover other unsupported expressions.
| ) | ||
| .ok()?; | ||
| let restored_plan = restore_join_schema(swapped_join, join.schema.clone())?; | ||
| let mut merge_scan = MergeScanLogicalPlan::new(restored_plan, false); |
There was a problem hiding this comment.
The admission-limit caveat in the PR description can make a single selected query unable to complete. With max_concurrent_queries = 1, if a datanode owns both an outer probe region and an inner build region, the outer request acquires the only permit in RegionServer::handle_remote_read_inner and retains it through PermitGuardedStream. Executing this nested plan then sends another read to the same datanode; that read waits for the permit, while the outer stream needs its result before it can finish and release the permit. The query therefore fails when the limiter wait times out. Larger limits have the same problem when outer streams occupy all permits.
Could we prevent this rewrite when nested reads cannot make progress under admission control, or provide bounded parent/child admission that avoids the wait cycle? Please add a same-node regression with a positive limit (especially 1), plus a saturated multi-node case. The existing tests use the default disabled limiter and do not cover this configuration.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Uses the nested datanode scan capability from #9206, now merged. This branch is synchronized with upstream main
28ec3ff5aad8d274d83ae42f96dc54f838fb6b20by ordinary merge; the earlier synchronization point wasb913be2af77a5a936bcccb3d5b50358d1313a777.What's changed and what's your intention?
Add an opt-in, default-off
experimental_dist_joinpath for supported two-scan INNER equijoins. Broadcast the smaller input to the larger table's datanodes instead of joining both remote scans on the frontend.JoinSelectionremains in control; there is no forced global input order.Inexact; memory and column statistics remain unknown. Disk size is a placement heuristic, not a hash-memory estimate. Unknown or unusable statistics leave planning unchanged.Limits: Query admission is unchanged. Positive
max_concurrent_queriescan cause nested requests to time out while outer streams hold permits; default-disabled tests do not establish finite-quota success. Typed read preference is preserved across supporting receivers; old receivers ignore its internal carrier. This change does not introduce enterprise bootstrap or automatic follower lifecycle. Earlier release/ECS measurements used an older BIG-build implementation and are not performance evidence for this SMALL-build implementation. A fresh release/ECS run on this implementation is reported below.Earlier verification on parent
1f22baf446d7ac94f1178dcdad6a4a85dad8e191(before the latest main synchronization):cargo check --locked -p query -p operator -p session -p meta-client -p datanode -p flow -p servers -p tests-integration --tests— passed.cargo nextest run --locked -p query -p operator -p session -p common-session -p meta-client --lib --retries 0 -j 6 --no-fail-fast— 1,118 passed, 2 existing skips, no retries.cargo nextest run --locked -p tests-integration --lib --retries 0 -j 1 --no-fail-fast -E 'test(nested_merge_scan_capability_test) | test(test_flow_requester_health_check_and_handle_request)'— 6 passed, including real SQL planning/execution and nested whole-stream/strict-ingress failure checks.cargo nextest run --locked -p cmd --features dev-tools --retries 0 -j 4 --no-fail-fast— 150 passed, 0 skipped, no retries.cargo nextest run --locked -p servers -p datanode --lib --retries 0 -j 2 --no-fail-fast -E 'test(grpc::greptime_handler::tests) | test(http::hints::tests) | test(http::read_preference::tests) | test(test_region_server_parallelism)'— 8 selected tests passed, no retries; 690 other tests were excluded by the filter, not run as a full suite.cargo clippy --locked -p cmd --features dev-tools --all-targets -- -D warnings— passed.cargo build --locked -p cmd --features dev-tools --bin greptime --bin query_perf_fixture --bin query_regression_runner -p sqlness-runner --bin sqlness-runner— passed. The freshly built server reports this exact commit andclean: true; all four binary hashes were recorded and verified.sqlness-runner bare -t '^(standalone|distributed):(create|view|dist_join)$' -c tests/cases --bins-dir "$CARGO_TARGET_DIR/debug"— 7/7 passed: common create, view/create and view/view in both environments, plus distributed explain/dist_join. All runner-owned cluster processes stopped;git diff --exit-code -- tests/casesconfirms generated expectations did not change. Final workspace formatting and diff checks passed.cargo clippy --locked -p query -p operator -p session -p meta-client -p datanode -p flow -p servers -p substrait@1.4.0-alpha.0 -p tests-integration --all-targets -- -D warnings— passed.The helper/protocol/parallelism regressions, four freshly built binaries and all seven real SQLness cases passed on the earlier clean parent
1f22baf446d7ac94f1178dcdad6a4a85dad8e191(details above), not on the latest re-synchronized head. Draft-skipped jobs are not functional test passes. The checked-in SQL test retains off→on→off plans and all five ordered six-column rows. The executed real SQL integration checks all 4,877 projected rows against frontend and known references, full schema, actual datanode SMALL hash-build/BIG probe metrics, and BIG-region pruning.Enterprise companion validation before the latest public re-sync: The previously synchronized public source and enterprise companion were revalidated in an isolated checkout. This combination has not been revalidated against the latest public head. All 10 selected nested/wire/Region/Flow tests passed without retries, using the normal enterprise pre-build hook and
DatanodeBuilder. Real outer/inner TCP Flight strict-Follower reads returned the complete known/frontend result; absence of followers failed the complete stream with no nested Leader fallback. Four supported typed-preference protobuf roundtrips, strict ingress/inner-failure checks, and the actual SQL broadcast JOIN's full rows/schema/build-probe roles/pruning passed. The strict Flow caller regression and six-related-crate all-target lint, formatting and locked checks also passed. Production source is unchanged by the validation-only test overlay; the formal enterprise companion pins upstream main containing merged #9206, not an unmerged feature or a private test commit. A freshly built formal enterprise binary also passed a separate real metasrv/datanode/frontend CLI run: normal pre-build hook, exact complete three-column/three-row SQL, matching clean build identity in all three startup logs, and successful cleanup of all three owned processes. Physical followers are opened explicitly by the focused fixture; the ordinary CLI check and the focused TCP Follower check are separate evidence. Automatic follower lifecycle and a single CLI Follower end-to-end scenario are not claimed.Latest main-sync status on
38f91de408d: The branch accepts upstream #9491's removal of MergeScan hash-partitioning advertisement (try_with_new_distributionandpartition_colsplumbing), retaining the logical broadcast JOIN rewrite, row estimates, producer identities, captured filters and per-region metrics. The eight-crate locked check and selected library/nested JOIN/Flow test chain completed successfully after the merge; the earlier library-test total above has not been independently re-counted for this head. Strict affected-crate Clippy completed before the formatting check failed; formatting was then corrected and workspace formatting plus strict query all-target Clippy passed. Independent read-only follow-up review approved the final source and confirmed closure of the constructor-arity, unused-binding and producer-ID coverage findings.Latest-head runtime verification: Latest head
47f3b2998b1passed the full non-Draft CI (Integration CI run38349416691and Rust CI run38349416607, bothcompleted/success): all five Sqlness suites, all four Fuzz Chaos jobs, all unit/integration tests green. This head includes the P1 volatile-expression guard and P2 wire-flag propagation fixes from review. The older paragraphs below still record earlier-parent evidence: the post-merge seven-case invocation used the previously built server, so it is not validation of those heads. Helper/protocol suite counts, fresh-binary evidence and the performance result below remain evidence for the earlier parent only. Generated expectations have not been changed by this re-sync; existing generated-result EOF blank-line warnings from range diff checks are not described as a clean full-range diff check.Release/ECS performance measured on earlier parent
1f22baf446d7ac94f1178dcdad6a4a85dad8e191: The existing query-regression workflow was dispatched once with immutable baseb913be2af77a5a936bcccb3d5b50358d1313a777(upstream main) and candidate1f22baf446d7ac94f1178dcdad6a4a85dad8e191on a dedicatedecs.u1-c1m1.2xlargerunner (qreg-ecs-38042793447), release profile, casetests/perf/query_cases/sql_join_dist_broadcast/case.toml. The run succeeded. The case query enablesexperimental_dist_joinand runs the fact/dimension broadcast join with aggregation and top-10 ordering. Results: base median 193.17 ms / p95 207.42 ms; candidate median 54.00 ms / p95 55.78 ms (-72.0%, thresholdmax_candidate_latency_regression_pctpassed, 0 validation errors). All three timed samples per side returned the identical eight orderedhost/zone/scorerows (maximum absolute score difference1.4e-12); theexecuted_plan_preflightcheck returned status 200. This is the current SMALL-hash-build implementation's own measurement, not the earlier BIG-build numbers.PR Checklist
backport-<target>labels (e.g.backport-v1.3targetsrelease/v1.3).